WIP: Feature: Friends & User Presence #33
Conversation
…eplace callbacks with futures
elkangaroo
force-pushed
the
feature/friends_presence
branch
from
fbd9747 to
4f43411
Compare
|
I rebased the branch and rewrote the socked handling using asyncio. |
|
Now friends presence works correctly when multiple "game_accounts" are returned (which - from my understanding - means the friend is logged in multiple times [on multiple devices]). Needs more testing, but otherwise I am quiet happy with the state of this PR now :) |
bartok765
left a comment
bartok765
left a comment
There was a problem hiding this comment.
Thanks for getting me read this tonight, it is nice to read good code and learn something.
I've added some comments and some questions but in fact mostly testing is needed.
| request = authentication_service_pb2.LogonRequest() | ||
| # request.program = "App" | ||
| request.program = "BSAp" # login in as mobile | ||
| request.platform = "Win" |
There was a problem hiding this comment.
What do you mean?
There was a problem hiding this comment.
I mean is platform="Win" (windows) valid if program stands for Mobile App? Shouldn't be Android or iOS instead?
I know that for now it works, but it may looks... strange for blizzard backend metrics...
There was a problem hiding this comment.
Ah, I see. I can try different values but don't know what it expects.
| # request.program = "App" | ||
| request.program = "BSAp" # login in as mobile | ||
| request.platform = "Win" | ||
| request.locale = "enUS" |
There was a problem hiding this comment.
for now we have no regional info fetched from proto communication so there is no need to specify that right?
There was a problem hiding this comment.
I tried not sending locale, but then get immediately disconnected.
Do we have the correct value from authentication that we could use? I think we only have "region" ('eu', 'us', ...) so far, right?
There was a problem hiding this comment.
yes, and the region is partially guessed, as login page require knowing it a priori...
There is https://www.blizzard.com/user but it defaults to en_US for me even if I have different settings (however I know it works for china).
For now I would leave en_US. Then we could ask @wanze for testing the same with china locale - if the rich present text is localized.
|
Thanks @bartok765 for your review. I will look into your comments now. |
|
Hey @bartok765 I addressed most of your comments, can you please review again? |
bartok765
left a comment
bartok765
left a comment
There was a problem hiding this comment.
I've left some comments, but it looks really nice already!
| # request.program = "App" | ||
| request.program = "BSAp" # login in as mobile | ||
| request.platform = "Win" | ||
| request.locale = "enUS" |
There was a problem hiding this comment.
yes, and the region is partially guessed, as login page require knowing it a priori...
There is https://www.blizzard.com/user but it defaults to en_US for me even if I have different settings (however I know it works for china).
For now I would leave en_US. Then we could ask @wanze for testing the same with china locale - if the rich present text is localized.
| request = authentication_service_pb2.LogonRequest() | ||
| # request.program = "App" | ||
| request.program = "BSAp" # login in as mobile | ||
| request.platform = "Win" |
There was a problem hiding this comment.
I mean is platform="Win" (windows) valid if program stands for Mobile App? Shouldn't be Android or iOS instead?
I know that for now it works, but it may looks... strange for blizzard backend metrics...
| await self._send_message(self._PRESENCE_SERVICE, 4, request, functools.partial(self._on_presence__query_account, future)) | ||
|
|
||
| async def fetch_friend_presence_account_details(self, account_id, future: asyncio.Future): | ||
| async def fetch_friend_presence_account_details(self, entity_id, future: 'asyncio.Future[AccountInfo]'): |
There was a problem hiding this comment.
we can avoid repeating code for fetch_friend_battle_tag and fetch_friend_presence_account_details
There was a problem hiding this comment.
What is your suggestion @bartok765 ? Maybe just put this in a private function with variable values replaced?
request = presence_service_pb2.QueryRequest()
request.entity_id.high = entity_id.high
request.entity_id.low = entity_id.low
for i in [...]:
key = request.key.add()
key.program = 0x424e # hex code for "BN"
key.group = ...
key.field = i
Or do you have another idea?
There was a problem hiding this comment.
group is the same in both methods, so yes - it could be either one private method for basic request with additional parameter of field list to set OR both methods may be just merged into one with additional param. I have no opinion which is better.
2 participants
WORK IN PROGRESS
Status Quo:
Partly solves #28
Needs more testing!